-
Notifications
You must be signed in to change notification settings - Fork 56
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix duplicate labels on the TimeSeriesChart x-axis #443
Conversation
"month": [...Array(getDaysInMonth(new Date()) + 1).keys()].map(index => subDays(new Date(), index)).reverse(), | ||
"week": [...Array(8).keys()].map(index => subDays(new Date(), index)).reverse(), | ||
"day": [...Array(25).keys()].map(index => subHours(new Date(), index)).reverse(), | ||
"hour": [...Array(61).keys()].map(index => subMinutes(new Date(), index)).reverse(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are these numbers arbitrary or where do they come from?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe I should have explained this.
To prevent duplicate labels I set the ticks manually.
The hour chart should have a maximum of 61 ticks, meaning 1 for every minute in an hour plus one more so the first and last ticks have the same time.
Likewise, the day chart should have 25 ticks since there are 24 hours in a day + 1 hour for the first tick.
It works analogously for the week and the month chart.
@@ -36,8 +36,33 @@ class TimeSeriesChart extends React.Component { | |||
resolution, | |||
} = this.props; | |||
|
|||
console.log(data); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove this.
Thanks for working on this, @benv2 😃👍 |
Make sure you run |
Set domain to auto as manual setting was causing errors. Removed code console.log mentioned in code review.
I fixed the linting violations and improved the code a bit. |
month: [...new Array(getDaysInMonth(new Date()) + 1).keys()].map(index => subDays(new Date(), index)).reverse(), | ||
week: [...new Array(8).keys()].map(index => subDays(new Date(), index)).reverse(), | ||
day: [...new Array(25).keys()].map(index => subHours(new Date(), index)).reverse(), | ||
hour: [...new Array(61).keys()].map(index => subMinutes(new Date(), index)).reverse(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nitpick, but it's wasteful to create so many date objects. I would define const now = new Date();
above const ticks
and use that.
You could also abstract some of the logic into a helper function:
const makeTicks = (count, fn) => {
return [...new Array(count).keys()].map(index => fn(new Date(), index)).reverse();
};
@@ -38,6 +38,17 @@ class TimeSeriesChart extends React.Component { | |||
|
|||
const labelFormat = resolutionToLabelFormat.get(resolution); | |||
|
|||
const getTicks = resolution => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can move this to the top-level, below the resolutionToLabelFormat
function, since it doesn't depend on any internal state.
As you suggested I changed the multiple new dates to a constant. This makes this possible: const getTicks = resolution => {
const ticks = {
year: makeTicks(13, 'month'),
month: makeTicks(getDaysInMonth(now) + 1, 'day'),
week: makeTicks(8, 'day'),
day: makeTicks(25, 'hour'),
hour: makeTicks(61, 'minute'),
};
... I hope this makes it easier to understand what the code does. |
day: (date, index) => subDays(date, index), | ||
hour: (date, index) => subHours(date, index), | ||
minute: (date, index) => subMinutes(date, index), | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't really see the point of this. It just makes it more verbose. makeTicks(13, 'month')
is not much clearer than makeTicks(13, subMonths)
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My bad. I forgot I could write it like that.
I passed (date, index) => subDays(date, index)
instead of subDays
which made it look relatively messy.
Will fix this!
@@ -25,6 +25,29 @@ const resolutionToLabelFormat = new Map([ | |||
['all', 'MMMM YYYY'], | |||
]); | |||
|
|||
const now = new Date(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs to be inside the makeTicks
function as we need the date when getTicks
called, not when the module is evaluated.
This is looking great! We really appreciate the help, @benv2 🙌 |
Weird. I can't reproduce that. Maybe it depends on when in the day you check. If you still see this, would you be interested in looking into it? |
Thank you for the guidance @sindresorhus .
I'll try to reproduce it and see what I can do. This is a bit off-topic but I was wondering if you still need help translating the app. |
Definitely, that would be great! We still have some untranslated strings in German and we expect more in the future. I've sent you an invite to our Crowdin project (It's what we use to translate the app). Feel free to look through the existing translated strings too and see if you find any mistakes or improvements ;) |
@sindresorhus Cool. Will do! 👍 |
Did my best to fix #311
I'm pretty new to open source so I'm not entirely sure if I'm doing this right.